Plan: Close CoderControlPlane controller best-practice gaps

Context / Why

The CoderControlPlane controller works, but the audit identified operational and test gaps that make it harder to run safely in production (HA), harder to debug, and easier to regress:

• Controller manager does not use leader election (unsafe if scaled to >1 replica).
• /readyz and /healthz are both healthz.Ping (readiness doesn’t reflect cache/API readiness).
• CoderControlPlane tests cover creation and defensive nil checks, but miss spec updates, status assertions, and key error paths.
• Defaults are applied imperatively in reconcilers; there’s no admission-time/CRD defaulting.

Goal: harden the controller app + reconciler and expand tests so the behavior is safer and the contract is verified.

Evidence (what was inspected)

• internal/app/controllerapp/controllerapp.go
  • Manager options only set Scheme + HealthProbeBindAddress (no LeaderElection).
  • Both health + ready checks use healthz.Ping.
• internal/controller/codercontrolplane_controller.go
  • Defaults for image/replicas/service type+port are applied inside reconcile.
  • Status update sets ObservedGeneration, ReadyReplicas, URL, Phase.
• internal/controller/codercontrolplane_controller_test.go
  • Tests: NotFound, create children, nil client/scheme.
  • Missing: spec update, status persistence, owner refs, error paths.
• internal/controller/suite_test.go
  • Uses controller-runtime envtest with CRDs loaded from config/crd/bases.
• Reference pattern: internal/controller/workspaceproxy_controller_test.go contains richer assertions (including status).

Implementation details

1) Enable leader election for the controller manager

Why: safe HA operation; prevents multiple controllers racing on the same resources.

Edits

• internal/app/controllerapp/controllerapp.go
  • Enable leader election in ctrl.Options.
  • Set a stable LeaderElectionID.
  • Prefer LeaderElectionReleaseOnCancel: true for faster handoff on shutdown.

Suggested shape:

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
    Scheme: scheme,
    HealthProbeBindAddress: HealthProbeBindAddress,

    LeaderElection: true,
    LeaderElectionID: "coder-k8s-controller.coder.com",
    LeaderElectionReleaseOnCancel: true,
})

RBAC (required if leader election is enabled)

• Add kubebuilder RBAC marker somewhere included by controller-gen (e.g., internal/app/controllerapp/controllerapp.go):

// +kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;update;patch;delete

• Regenerate manifests (make manifests) so config/rbac/role.yaml includes leases permissions.
• Verify any install manifests (deploy/rbac.yaml, config/e2e/*) bind the controller SA to the regenerated role (or update them if they’re intended to be used).

2) Make readiness meaningful (cache sync instead of Ping)

Why: Kubernetes should only route traffic / treat the controller as ready once caches are started and synced.

Edits

• internal/app/controllerapp/controllerapp.go
  • Keep liveness as healthz.Ping.
  • Replace ready check with a cache-sync check (wrap mgr.GetCache().WaitForCacheSync). Ensure it does not hang indefinitely by using a short timeout.

Suggested shape:

if err := mgr.AddReadyzCheck("cache-sync", healthz.Checker(func(req *http.Request) error {
    ctx, cancel := context.WithTimeout(req.Context(), 2*time.Second)
    defer cancel()

    if ok := mgr.GetCache().WaitForCacheSync(ctx); !ok {
        return fmt.Errorf("cache not synced")
    }
    return nil
})); err != nil {
    return fmt.Errorf("unable to set up ready check: %w", err)
}

3) Move defaults out of “hidden” controller logic (prefer CRD defaults)

Why: users and automation should be able to see the effective defaults via kubectl get -o yaml and avoid surprise behavior.

Preferred approach (low overhead): CRD defaults via kubebuilder markers

• Add +kubebuilder:default markers to API types.
  • api/v1alpha1/codercontrolplane_types.go
    • CoderControlPlaneSpec.Image default ghcr.io/coder/coder:latest
    • CoderControlPlaneSpec.Replicas default 1
  • api/v1alpha1/types_shared.go
    • ServiceSpec.Type default ClusterIP
    • ServiceSpec.Port default 80

Example:

// +kubebuilder:default="ghcr.io/coder/coder:latest"
Image string `json:"image,omitempty"`

// +kubebuilder:default=1
Replicas *int32 `json:"replicas,omitempty"`

• Run make manifests to update CRDs under config/crd/bases/.
• Keep reconciler-level defaults for backward compatibility (old CRDs), but add a TODO to remove once defaults are guaranteed by CRDs.

4) Expand envtest coverage for CoderControlPlane

Why: prevent regressions in update behavior and status reporting.

Edits

• internal/controller/codercontrolplane_controller_test.go

Add tests mirroring the thoroughness of workspaceproxy_controller_test.go:

1. Status is persisted after reconcile

   • After reconcile, Get the CoderControlPlane and assert:
     • Status.ObservedGeneration == Generation
     • Status.URL == fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", ...)
     • Status.ReadyReplicas == 0 (in envtest)
     • Status.Phase == Pending

2. Owner references are set (in lieu of GC tests)

   • Assert Deployment.OwnerReferences and Service.OwnerReferences include the CP with controller=true.

3. Spec updates propagate

   • Update CP spec fields (replicas, image, service port/annotations), call reconcile, assert Deployment/Service updated.

4. Status transitions to Ready when deployment is “ready”

   • Manually set deployment.Status.ReadyReplicas = 1 via k8sClient.Status().Update.
   • Reconcile again and assert CP Status.Phase == Ready and ReadyReplicas == 1.

5. Defaulting behavior is covered

   • Create a CP with minimal/empty spec, reconcile, and assert defaults took effect (either by checking the stored spec fields if using CRD defaults, and/or checking reconciled child resources).

5) Add targeted error-path tests (unit-style)

Why: ensure reconcile wraps and returns actionable errors (especially around status writes).

Approach:

• Add a thin client.Client wrapper used only in tests to force failures on:
  • Status().Update (to cover update control plane status: ... path)
  • optionally Create for Deployments/Services (to cover reconcile child errors)

This avoids needing a full fake API server while still verifying error-handling behavior.

6) Validation / regeneration checklist

When implementing:

• make manifests (CRD + RBAC generation)
• make test
• make lint
• (optional) make build

Expected outcome

• Controller manager supports HA safely (leader election + required RBAC).
• Readiness indicates real operational readiness (cache synced).
• Defaults are visible and consistent (CRD defaults preferred).
• CoderControlPlane tests cover create/update/status/ownership/error paths, reducing regressions.